Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cross-build for memcg notification #37384

Merged
merged 1 commit into from
Nov 23, 2016

Conversation

derekwaynecarr
Copy link
Member

@derekwaynecarr derekwaynecarr commented Nov 23, 2016

Fix the cross-build error from #32577

Fixes #37340

Fixes #37383


This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 23, 2016
@derekwaynecarr
Copy link
Member Author

fyi @sjenning @rmmh @mikedanese @dchen1107 @ncdc

I am testing this now, will report back if successful.

@derekwaynecarr
Copy link
Member Author

fyi @saad-ali to fix build.

@derekwaynecarr derekwaynecarr added this to the v1.6 milestone Nov 23, 2016
@derekwaynecarr derekwaynecarr modified the milestones: v1.5, v1.6 Nov 23, 2016
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Nov 23, 2016
@derekwaynecarr derekwaynecarr added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Nov 23, 2016
@derekwaynecarr
Copy link
Member Author

@pires -- i was able to build on windows.

$ KUBE_BUILD_PLATFORMS=windows/amd64 make WHAT=cmd/kubelet
+++ [1123 11:40:10] Generating bindata:
    /home/decarr/go/src/k8s.io/kubernetes/test/e2e/framework/gobindata_util.go
+++ [1123 11:40:11] Building the toolchain targets:
    k8s.io/kubernetes/hack/cmd/teststale
+++ [1123 11:40:11] Building go targets for windows/amd64:
    cmd/libs/go2idl/openapi-gen
+++ [1123 11:41:04] Generating bindata:
    /home/decarr/go/src/k8s.io/kubernetes/test/e2e/framework/gobindata_util.go
+++ [1123 11:41:05] Building the toolchain targets:
    k8s.io/kubernetes/hack/cmd/teststale
+++ [1123 11:41:05] Building go targets for windows/amd64:
    cmd/kubelet

@pires
Copy link
Contributor

pires commented Nov 23, 2016

Thanks @derekwaynecarr!

@derekwaynecarr
Copy link
Member Author

ok, this should go all green now that i updated the test owners file.

@mikedanese
Copy link
Member

mikedanese commented Nov 23, 2016

I'd like @rmmh to confirm that cross-builds were failing because of kubelet on windows and not on non amd64 linux archs. I can't find the build output anywhere or I would check.

@rmmh
Copy link
Contributor

rmmh commented Nov 23, 2016

@mikedanese it's in #37340, here's an example failure. It's hard to determine exactly which flavor of builds were failing.

@rmmh
Copy link
Contributor

rmmh commented Nov 23, 2016

also, you can trigger a cross-build like this:

@k8s-bot build this

@ncdc
Copy link
Member

ncdc commented Nov 23, 2016

That example shows both darwin and windows failures. LGTM if everyone else is ok

@derekwaynecarr
Copy link
Member Author

@rmmh -- thanks for the magic invocation on cross-build trigger. that should clarify things.

@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit 609877e. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@derekwaynecarr
Copy link
Member Author

@mikedanese -- should i pull a new version of gazel?

@@ -16,6 +16,7 @@ go_library(
"doc.go",
"eviction_manager.go",
"helpers.go",
"threshold_notifier_unsupported.go",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should drop the change to this file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block should only contain files used to build linux, amd64. We don't need this file.

@mikedanese
Copy link
Member

@derekwaynecarr are you on mac or linux? did you get that diff by running ./hack/update-bazel.sh?

@mikedanese
Copy link
Member

mikedanese commented Nov 23, 2016

Every incantation of ./hack/update-bazel.sh should pull a new version of gazel. If you are calling gazel directly then yes please pull new versions fairly often.

@derekwaynecarr
Copy link
Member Author

@mikedanese - i was on fedora 24 and got that diff.

@derekwaynecarr
Copy link
Member Author

@rmmh @mikedanese -- it looks like cross build is happy again with this, i will try and re-run bazel update to make jenkins happy.

@derekwaynecarr
Copy link
Member Author

pushed one last update for bazel, it's possible that diff was from an earlier commit where build tag was wrong on the file. either way, this should pass cross build per the last version i sent.

@k8s-bot build this

i am not going to be available for next few hours, but this should be good to go.

@derekwaynecarr
Copy link
Member Author

Ok. All the things are happy.

@ncdc ncdc added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 23, 2016
@pires
Copy link
Contributor

pires commented Nov 23, 2016

@ncdc this fixes P0 stuff so is it needed to have P0 label as well?

@ncdc ncdc added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Nov 23, 2016
@ncdc
Copy link
Member

ncdc commented Nov 23, 2016

@pires labeled

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE etcd3 e2e failed for commit 1ec69f6. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@ncdc
Copy link
Member

ncdc commented Nov 23, 2016

@k8s-bot gce etcd3 e2e test this

On Wed, Nov 23, 2016 at 2:08 PM k8s-ci-robot notifications@github.com
wrote:

Jenkins GCE etcd3 e2e failed
https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/37384/pull-kubernetes-e2e-gce-etcd3/5903/
for commit 1ec69f6
1ec69f6.
Full PR test history http://pr-test.k8s.io/37384.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e
test this. Please help us cut down flakes by linking to an open flake
issue
https://github.com/kubernetes/kubernetes/issues?q=is:issue+label:kind/flake+is:open
when you hit one in your PR.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#37384 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAABYht858h1c-5S46V3-tQjXFFzVFObks5rBI8UgaJpZM4K6wqd
.

@dchen1107
Copy link
Member

LGTM and I am manually merged this one for build fix.

@dchen1107 dchen1107 merged commit e968173 into kubernetes:master Nov 23, 2016
@saad-ali
Copy link
Member

Removing cherry-pick candidate label this is already in 1.5

gmarek added a commit to gmarek/kubernetes that referenced this pull request Dec 7, 2016
…ross-build"

This reverts commit e968173, reversing
changes made to 5190ace.
k8s-github-robot pushed a commit that referenced this pull request Sep 3, 2017
Automatic merge from submit-queue (batch tested with PRs 51666, 49829, 51058, 51004, 50938)

Fix threshold notifier build tags

**What this PR does / why we need it**:
Cross building from darwin is currently broken on the following error:
```
# k8s.io/kubernetes/pkg/kubelet/eviction
pkg/kubelet/eviction/threshold_notifier_unsupported.go:25: NewMemCGThresholdNotifier redeclared in this block
        previous declaration at pkg/kubelet/eviction/threshold_notifier_linux.go:38
```
It looks like #49300 broke the build tags introduced in #38630 and #37384. This fixes the build tag on `threshold_notifier_unsupported.go` as the cgo requirement was removed from `threshold_notifier_linux.go`.

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #50935

**Special notes for your reviewer**:

**Release note**:
```release-note
NONE
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants